-
Notifications
You must be signed in to change notification settings - Fork 514
Support alsa plugins for sound using pulseaudio/alsa-lib #2984
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Conversation
097a1d4 to
2d238d9
Compare
fix boilerplate violations, license, maintainer, etc
2070773 to
7cfb60a
Compare
7cfb60a to
f81995a
Compare
|
|
||
| license("LGPL-2.1-or-later") | ||
|
|
||
| version("1.2.15.2", tag="v1.2.15.2", commit="63a981865a1c7d9501ae556e28ae3bb53d015b61") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a particular reason to use a tag here? That prevents caching the source.
|
|
||
| homepage = "https://www.alsa-project.org" | ||
| url = "ftp://ftp.alsa-project.org/pub/lib/alsa-lib-1.2.3.2.tar.bz2" | ||
| url = "ftp://ftp.alsa-project.org/pub/lib/alsa-lib-1.2.15.2.tar.bz2" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just as an FYI, it is not necessary to update the url each time a new version is added. Spack determines this from older urls automatically.
| depends_on("autoconf", type="build", when="@1.2.15.2 build_system=autotools") | ||
| depends_on("automake", type="build", when="@1.2.15.2 build_system=autotools") | ||
| depends_on("libtool", type="build", when="@1.2.15.2 build_system=autotools") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems surprising. Why would only this particular version depend on these packages? And what other option for build_system is there?
|
|
||
| license("LGPL-2.1-or-later", checked_by="biddisco") | ||
|
|
||
| version("1.2.12", tag="v1.2.12", commit="52574cb5ccbb8b546df2759e4b341a20332269b6") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same question here about using a git tag instead of a download.
| depends_on("alsa-lib") | ||
| depends_on("pulseaudio +alsa", when="+pulseaudio") | ||
|
|
||
| variant("pulseaudio", default=True, description="Enable pulseaudio support") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Often it is easier to set the default as False, except for really indispensable functionality.
| # if spec.satisfies("%gcc@8:"): | ||
| os.environ["LDFLAGS"] = "-Wl,--copy-dt-needed-entries" | ||
|
|
||
| def configure_args(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is safer to add the configure_args and meson_args to separate builders, e.g.
| class AutotoolsBuilder(autotools.AutotoolsBuilder): |
|
|
||
| def setup_build_environment(self, env): | ||
| if self.spec.satisfies("build_system=meson"): | ||
| env.append_flags("CPPFLAGS", "-I{0}".format(self.spec["libiconv"].prefix.include)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be CXXFLAGS? CPP is for the preprocessor.
I am also surprised these are necessary if you depend on libiconv. Why does the build not find the right include directory?
| return [ | ||
| "-Ddatabase=gdbm", | ||
| "-Ddoxygen=false", | ||
| "-Dbluez5=disabled", | ||
| "-Dx11=disabled", | ||
| "-Dtests=false", | ||
| "-Ddefault_library=shared", | ||
| "-Dprefix={0}".format(self.prefix), | ||
| "-Dlibdir={0}/lib64".format(self.prefix), | ||
| "-Dbashcompletiondir={0}/share/bash-completion/completions".format(self.prefix), | ||
| "-Dsystemduserunitdir={0}/lib/systemd/user".format(self.prefix), | ||
| ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We typically try to make sure that two build systems of the same package at least in spirit behave the same way. In the case of configure, you have enable dbus and glib2, but that's not explicitly added for meson. Can you verify that?
|
|
||
| # If pulseaudio is in the spec, make sure its libraries are on the runtime linker path | ||
| if self.spec.satisfies("^pulseaudio"): | ||
| # 1️⃣ Standard lib64 dir |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try to stick to regular ascii charsets.
| # 2️⃣ Also check for a pulseaudio subdirectory (some builds install here) | ||
| pulseaudio_libdir = join_path(self.spec["pulseaudio"].prefix.lib64, "pulseaudio") | ||
| if os.path.exists(pulseaudio_libdir): | ||
| env.prepend_path("LD_LIBRARY_PATH", pulseaudio_libdir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as earlier about LD_LIBRARY_PATH.
This adds a new alsa-plugins package and upgrades the existing alsa-lib and pulseaudio packages so that they work with applications such as musescore https://github.com/musescore/MuseScore